Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename 'host' bean parameter to 'bean_host' in tags #59

Merged
merged 2 commits into from
May 28, 2015

Conversation

olivielpeau
Copy link
Member

@yannmh Fixes the case when a bean parameter named 'host' overrides the actual 'host' tag.

@yannmh
Copy link
Member

yannmh commented May 26, 2015

Looks great, thanks @olivielpeau !

Shall we merge it for the 5.4 agent release ? cc @remh

@olivielpeau
Copy link
Member Author

@remh This PR shoud be mergeable, should I open a PR on https://github.com/DataDog/documentation to document the change ?

@remh
Copy link

remh commented May 27, 2015

It would be simpler and more efficient to just check the value of the key here and replace it if needed:
https://github.com/DataDog/jmxfetch/blob/olivielpeau/rename-host-parameter/src/main/java/org/datadog/jmxfetch/JMXAttribute.java#L84

@olivielpeau
Copy link
Member Author

I've thought about it but one problem I had with it is that it would change not only the name of the tag that is sent, but also the name of the bean parameter itself in JMXAttribute, and that could lead to confusing things when we try to match the JMXAttribute with the configuration.

That's why I did it in two steps: first deal with the bean parameters (and keep the host bean parameter unchanged) and then deal with the default tags (and change the name of the tag to bean_host).

Actually I think there are still a few confusing things about the way we don't clearly separate bean parameters from default tags. For instance I think the instanceTags shouldn't be added to the bean parameters list, but only to the default tags.

@@ -74,11 +74,13 @@ public void testBeanTags() throws Exception {
Set<String> tagsSet = new HashSet<String>(Arrays.asList(tags));

// We should find bean parameters as tags
assertEquals(4, tags.length);
assertEquals(5, tags.length);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you fix the indentation here please ?

@remh
Copy link

remh commented May 28, 2015

Good point.

Looks good to me besides the indentation issues. Can you fix them and then merge the PR ? Thanks!

💥

@olivielpeau
Copy link
Member Author

Fixed the indentation issues, merging!

olivielpeau added a commit that referenced this pull request May 28, 2015
Rename 'host' bean parameter to 'bean_host' in tags
@olivielpeau olivielpeau merged commit 9f36585 into master May 28, 2015
@olivielpeau olivielpeau deleted the olivielpeau/rename-host-parameter branch May 28, 2015 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants